-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tool 233 extend user agent on js network providers #67
Tool 233 extend user agent on js network providers #67
Conversation
src/NetworkProviderConfig.ts
Outdated
@@ -0,0 +1,5 @@ | |||
import { AxiosRequestConfig } from 'axios'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename should be with lower case, for consistency. Name of interface can also match the file name, interface NetworkProviderConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/apiNetworkProvider.ts
Outdated
|
||
constructor(url: string, config?: AxiosRequestConfig) { | ||
constructor(url: string, config?: ExtendedAxiosRequestConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/apiNetworkProvider.ts
Outdated
private backingProxyNetworkProvider; | ||
private userAgentPrefix = 'sdk-network-providers/api' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the near future, we will unify the JS libraries. Thus, let's use multiversx-sdk/api
(or something similar - "multiversx" is necessary, to stand out from other possible libraries that talk to MultiversX APIs).
Constants can also be moved to constants.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/proxyNetworkProvider.ts
Outdated
|
||
// TODO: Find & remove duplicate code between "ProxyNetworkProvider" and "ApiNetworkProvider". | ||
export class ProxyNetworkProvider implements INetworkProvider { | ||
private url: string; | ||
private config: AxiosRequestConfig; | ||
private config: ExtendedAxiosRequestConfig; | ||
private userAgentPrefix = 'sdk-network-providers/proxy' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, multiversx-sdk/proxy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/userAgent.ts
Outdated
import { AxiosHeaders } from "axios"; | ||
import { ExtendedAxiosRequestConfig } from "./NetworkProviderConfig"; | ||
|
||
export function setUserAgent(userAgentPrefix: string, config: ExtendedAxiosRequestConfig | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extendUserAgent
or something similar (we are not clearing what has been previously set)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a small unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, tests added
src/userAgent.ts
Outdated
import { AxiosHeaders } from "axios"; | ||
import { ExtendedAxiosRequestConfig } from "./NetworkProviderConfig"; | ||
|
||
export function setUserAgent(userAgentPrefix: string, config: ExtendedAxiosRequestConfig | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current calls of this function, config
is never undefined
. Thus, we can simplify the function signature (and the first lines of the implementation / dropping the first check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/userAgent.ts
Outdated
|
||
const headers = AxiosHeaders.from(config.headers as AxiosHeaders).normalize(true); | ||
if (!config.clientName) { | ||
console.log("Can you please provide the client name of the aplication that uses the sdk?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Can also be as advice instead of question e.g. "please provide ..., to be used for (shallow) analytics."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}; | ||
|
||
const headers = AxiosHeaders.from(config.headers as AxiosHeaders).normalize(true); | ||
if (!config.clientName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check can be above const headers = ...
(so that we have the checks nicely grouped above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/userAgent.ts
Outdated
if (!config.clientName) { | ||
console.log("Can you please provide the client name of the aplication that uses the sdk?") | ||
} | ||
const resolvedClientName = config.clientName || 'unknown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placeholder can be moved to constants.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/userAgent.ts
Outdated
const resolvedClientName = config.clientName || 'unknown'; | ||
|
||
const currentUserAgent = headers.hasUserAgent() ? headers.getUserAgent() : ''; | ||
const newUserAgent = `${currentUserAgent} ${userAgentPrefix}${resolvedClientName}`.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A space is missing between userAgentPrefix
and resolvedClientName
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/constants.ts
Outdated
@@ -3,3 +3,5 @@ import { Address } from "./primitives"; | |||
|
|||
export const MaxUint64AsBigNumber = new BigNumber("18446744073709551615"); | |||
export const EsdtContractAddress = new Address("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzllls8a5w6u"); | |||
export const MetricsPrefix = "multiversx-sdk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseUserAgent
(or something similar) instead of metricsPrefix
?
src/apiNetworkProvider.ts
Outdated
this.url = url; | ||
this.config = { ...defaultAxiosConfig, ...config }; | ||
this.backingProxyNetworkProvider = new ProxyNetworkProvider(url, config); | ||
const proxyConfig = JSON.parse(JSON.stringify(this.config)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about structuredClone()
, does it help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to use this I need to upgrade the types/node to a version higher then 17, maybe ww will do that with a major release
assert.equal(localProxyProvider.config.headers.getUserAgent(), expectedProxyUserAgent); | ||
}); | ||
|
||
it("should keep the set userAgent and add the sdk to it", async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
9f9e675
No description provided.